Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reduce ID length for new credentials #32

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Reduce ID length for new credentials #32

merged 1 commit into from
Oct 5, 2023

Conversation

robin-nitrokey
Copy link
Member

@robin-nitrokey robin-nitrokey commented Jul 10, 2023 •
edited
Loading

This patch implements the following changes to reduce the ID length for new credentials:

  • Rename the old Credential type to FullCredential and introduce a StrippedCredential type and a Credential enum to differentiate between full and reduced credential data.
  • Flatten the credential data to reduce encoding overhead.
  • Remove the RP id from the credential data to reduce the total length.
  • Add a marker field use_short_id to FullCredential so that we don’t change the credential ID for existing RKs.

To do:

  • Add tests for EncryptedSerializedCredential serialized length (should be less than 255).
  • Add tests for FullCredential serialized length (should fit into one littlefs block).
  • Investigate if we can remove more fields from StrippedCredential.
    • As we meet the size limit and the remaining fields only need one to four bytes, I think it’s not worth removing them with the risk that we might need them in the future. We can still drop them if there is need.
  • Investigate what to do for CTAP1 credentials.
    • Changed to use StrippedCredential in register (authenticate automatically handles both by using the Credential enum).

Fixes: #29

@robin-nitrokey robin-nitrokey mentioned this pull request Jul 10, 2023
Open
szszszsz
szszszsz approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@szszszsz szszszsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far

@szszszsz szszszsz self-requested a review July 10, 2023 21:45
robin-nitrokey
robin-nitrokey commented Jul 11, 2023
View reviewed changes
src/credential.rs Outdated Show resolved Hide resolved
@szszszsz
Copy link
Member

For the sake of coherency in GA - can't you just echo back the incoming credential ID, if validated?

@robin-nitrokey
Copy link
Member Author

robin-nitrokey commented Jul 11, 2023 •
edited
Loading

For resident keys, the sever does not have to send the credential ID. We could use it as an additional check for non-resident keys but I’m not sure if this is worth the implementation effort. (We already sort of do that for non-resident keys because the credential ID passed in the allow list determines if the credential is deserialized to a FullCredential (old) or to a StrippedCredential (new), which then influences the serialization format.)

@robin-nitrokey
Reduce ID length for new credentials
d318c11 
This patch implements the following changes to reduce the ID length for
new credentials:
- Rename the old Credential type to FullCredential and introduce a
  StrippedCredential type and a Credential enum to differentiate between
  full and reduced credential data.
- Flatten the credential data to reduce encoding overhead.
- Remove the RP id from the credential data to reduce the total length.
- Add a marker field use_short_id to FullCredential so that we don’t
  change the credential ID for existing RKs.

Fixes: Nitrokey#29
@robin-nitrokey robin-nitrokey marked this pull request as ready for review October 4, 2023 10:58
@robin-nitrokey
Copy link
Member Author

I think this should be ready to merge. Potentially, we could drop some more fields from the stripped credentials, but I don’t think that’s necessary atm.

@robin-nitrokey
Copy link
Member Author

Nitrokey/nitrokey-3-tests@a70cad3 regular and upgrade tests passed when using this version of fido-authenticator on top of Nitrokey/nitrokey-3-firmware#350.

@robin-nitrokey robin-nitrokey merged commit d318c11 into Nitrokey:main Oct 5, 2023
@robin-nitrokey robin-nitrokey deleted the cred-id branch October 5, 2023 18:48
@robin-nitrokey robin-nitrokey mentioned this pull request Nov 2, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daringer daringer daringer approved these changes

@sosthene-nitrokey sosthene-nitrokey sosthene-nitrokey approved these changes

@szszszsz szszszsz Awaiting requested review from szszszsz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reduce credential ID size
4 participants
@robin-nitrokey @szszszsz @daringer @sosthene-nitrokey